Skip to content

Fix floating point precision issue with "--bumpslopes" exporter #1913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

lecocqp
Copy link
Contributor

@lecocqp lecocqp commented Apr 10, 2018

This patch stores the slope values in (i,j) texture space rather than in normalized s,t space to prevent floating point precision issue when dealing with very high resolution textures.

This was particularly sensitive with half precision format where the square slopes can cause floating point overflow (because the width of 1 texel is 1.0 / res_x in (s,t) space).

To retrieve the slopes in normalized (s,t) space, we just need to scale the slopes with the texture resolution as follow:

dhds = dhds *res_x
dhdt = dhdt * res_y
dhds2 = dhds2 * res_x * res_x
dhdt2 = dhdt2 * res_y * res_y
dh2dsdt = dh2dsdt * res_x * res_y

This patch stores slopes values in i,j texture space rather than normalized s,t space to prevent
floating point overflow with half precision float format.

testsuite update
@lecocqp lecocqp changed the title Fix floating point precision issue on stored slope values. Fix floating point precision issue with "--bumpslopes" exporter Apr 10, 2018
@lgritz
Copy link
Collaborator

lgritz commented Apr 10, 2018

Just to be clear, the res is of the top level of the MIP-map, not that it's rescaled separately for each level, right?

@lecocqp
Copy link
Contributor Author

lecocqp commented Apr 10, 2018

Absolutely right!

@lgritz
Copy link
Collaborator

lgritz commented Apr 10, 2018

OK, LGTM, merging.

@lgritz lgritz merged commit 3380422 into AcademySoftwareFoundation:master Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants